-
Notifications
You must be signed in to change notification settings - Fork 502
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
exp/ticker: Orderbook data support #1193
Conversation
Please wait until #1204 is merged before reviewing, since the tweaked db migrations for that PR will need to be merged here (and a new |
Done! Safe to review now ✅ |
@@ -74,7 +74,8 @@ type Market { | |||
change: Float! | |||
close: Float! | |||
intervalStart: Time! | |||
firstLedgerCloseTime: Time! | |||
firstLedgerCloseTime: Time!, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think you want this ,
or the ones below
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! I've sent a fix for that.
exp/ticker/internal/main.go
Outdated
@@ -32,6 +32,14 @@ type MarketStats struct { | |||
Price float64 `json:"price"` | |||
Close float64 `json:"close"` | |||
CloseTime int64 `json:"close_time"` | |||
NumBids int `json:"num_bids"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thinking about it, maybe we should unify where Bid
appears in these names? So maybe:
BidCount
BidVolume
BidHighest
(or BidMax
)
and the same for Ask
variables
What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, that makes sense! I've pushed a change that updates all the related fields to have Bid
or Ask
in the beginning. I've also updated the GraphQL interface to reflect that.
r := horizonclient.OrderBookRequest{} | ||
|
||
switch bType { | ||
case string(horizonclient.AssetTypeNative): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems a little verbose, is it possible to just assign type, code, and issuer in all cases?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I agree it was a little verbose. We still needed some logic here, as we store "XLM" as the asset code for native, and the Horizon API requires the asset code to be empty when type = native
. I've updated the function and added some comments for context.
if err != nil { | ||
return obStats, errors.Wrap(err, "could not create a orderbook request") | ||
} | ||
summary, err := c.Client.OrderBook(r) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe it's possible to have multiple pages of offers, so I'd keep paging through if necessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've taken a look at the Go SDK and client.OrderBook
doesn't seem to return a Page
-like entity. I've also checked the official Horizon documentation and there are no references to a _links
field, so I'm guessing this specific endpoint isn't paginated. Is this some that was recently changed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, you're right. That's a little unfortunate. There is a limit field, though, so it's definitely possible to make a request and get less than full orderbook data. Maybe just set the limit to 200 for now as a hack fix? Can you also add an issue for Horizon to add full pagination to the orderbook endpoint?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done! I've just pushed a change that adds the 200 limit, and also created the Horizon issue #1230.
if err != nil { | ||
return errors.Wrap(err, "invalid bid price") | ||
} | ||
obStats.BidVolume += pricef |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should be obStats.BidVolume += bid.Amount
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done!
obStats.HighestBid = 0 | ||
} | ||
for _, bid := range summary.Bids { | ||
pricef, err := strconv.ParseFloat(bid.Price, 64) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd grab the price from bid.PriceR
instead, it'll be higher precision.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done!
obStats.LowestAsk = 0 | ||
} | ||
for _, ask := range summary.Asks { | ||
pricef, err := strconv.ParseFloat(ask.Price, 64) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use ask.PriceR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done!
if err != nil { | ||
return errors.Wrap(err, "invalid ask price") | ||
} | ||
obStats.AskVolume += pricef |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should be obStats.AskVolume += pricef * ask.Amount
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done! The fact that we're multiplying by the amount is related to #612, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
exactly, yep! (well, the fact that we're multiplying by price).
@@ -112,7 +129,14 @@ SELECT | |||
COALESCE(open_price_7d, 0.0) as open_price_7d, | |||
|
|||
COALESCE(last_price, 0.0) as last_price, | |||
COALESCE(last_close_time, now()) as close_time | |||
COALESCE(last_close_time, now()) as close_time, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, I'm not sure this is the right fallback if there are no close times. Could this ever happen? Maybe it should be 0 instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since it's a right join, there could be null 24h close times. I agree that now()
didn't make sense, so I've updated it to fall back to the 7d close time.
exp/ticker/internal/utils/main.go
Outdated
if askMin == 0 || bidMax == 0 { | ||
return 0, 0 | ||
} | ||
spread = math.Abs(askMin-bidMax) / askMin |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can remove the math.Abs
, a negative spread is impossible in Stellar. But if it did happen, it'd be nice to know about it anyway since a negative spread is well defined.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! Maybe I missed it, where's the logic that defaults 24h OHLC to the 7d close?
Just to document here – we've discussed this offline and I'll address the OHLC values issue (#1218) on a separate PR. |
* horizonclient/txnbuild README fixes (#1210) * fix client links in top-level readme * update clients README * mark old client deprecated in godoc short summary * fix code of conduct, standardise example * fix code of conduct/contributing links * txnbuild: enables multiple signatures (#1198) This PR enables multiple signatures on a transaction in the new Go SDK. It also lets every `Operation` type have a different source account than its `Transaction`. These changes are intertwined. Without multiple signatures, every operation in a transaction must share the transaction's source account. Differing source accounts are the most common use case for multiple signatures, and they also test it with greatest completeness. * root repo changelog links to sub-projects (#1214) * keystore: add update-keys endpoints to spec (#1215) We need an endpoint to update the encrypted seed when users forget their passwords. * move keystore to exp to fix build (#1223) * Add minimal files to fix build (#1225) * exp/ticker: Orderbook data support (#1193) * exp/ticker: update partial market query to also output base/counter asset types * exp/ticker: add functions to scrape orderbook data * exp/ticker: update orderbookstats param names to match market standards * exp/ticker: create orderbookstats database model and migrations * exp/ticker: consolidate upsert logic and implement orderbookstats upsert * exp/ticker: create CLI command to ingest orderbooks * exp/ticker: update horizon SDK path * exp/ticker: add orderbook stats to market.json generator * exp/ticker: add separate query to retrieve relevant markets * exp/ticker: add orderbook stats to graphql interface * exp/ticker: add Docker support for orderbook ingestion * exp/ticker: fix partial aggregated market query + add tests * exp/ticker: fix globall aggregated market query + add tests * exp/ticker: update ticker binary link and fix crontab comment * exp/ticker: fix issue that would enable inf values to be stored in db * exp/ticker: ensure only store valid orderbook entries are stored * exp/ticker: format gql/static/bindata.go to prevent CI errors * exp/ticker: add updated bindata with pg 9.5-compatible migrations * exp/ticker: ensure markets with 0 orderbook entries can be handled by tickerdb * exp/ticker: remove unnecessary commas from graphql schema * exp/ticker: consolidate bid and ask positions on field names * exp/ticker: simplify logic for creating orderbook requests * exp/ticker: fix how bid and ask volumes are calculated * exp/ticker: allow negative spread values * exp/ticker: fix close_time fallback on markets query * exp/ticker: add page size limit to orderbook requests * Make PR template a bit more helpful. (#1238) * Make PR template a bit more helpful. Clean up some typos, and point to an example docs folder in the go repo rather than just the external docs hosted on our site. * Fix typo. * Agh, formatting. * horizonclient: update Fund method (#1213) * update client.Fund * update change log * fix space * horizonclient: add more documentation (#1226) * Remove old stream method * more comments * fix format * split comment * horizonclient: Set default HTTP client (#1228) * set default HTTP client * changes from review * changelog for txnbuild 1.1 (#1231) * horizonclient/txnbuild README fixes (#1210) * fix client links in top-level readme * update clients README * mark old client deprecated in godoc short summary * fix code of conduct, standardise example * fix code of conduct/contributing links * txnbuild: enables multiple signatures (#1198) This PR enables multiple signatures on a transaction in the new Go SDK. It also lets every `Operation` type have a different source account than its `Transaction`. These changes are intertwined. Without multiple signatures, every operation in a transaction must share the transaction's source account. Differing source accounts are the most common use case for multiple signatures, and they also test it with greatest completeness. * root repo changelog links to sub-projects (#1214) * keystore: add update-keys endpoints to spec (#1215) We need an endpoint to update the encrypted seed when users forget their passwords. * move keystore to exp to fix build (#1223) * Add minimal files to fix build (#1225) * changelog for txnbuild 1.1 * update change log (#1233) * horizonclient: add version (#1229) * add package version * fix go fmt * clients/horizonclient: support dynamic effects (#1217) * update effects in protocols/horizon * update client interface * update tests * update changelogs * fix typo
This PR:
It aims to:
markets.json
file that is constantly refreshed